Improve stability of package and tests #126

jacekbogdanski · 2020-08-03T06:55:49Z

I've fixed some tests so the whole test suite is more stable now. Also, I've corrected the implementation of getEditorNamespace because:

in case of error, it didn't clean up the promise
in case of empty string, it directly threw an error, which is not consistent with promise-based functions (you shouldn't have to decorate promised function with try/catch block)

The tests are still failing because of

ckeditor4-angular/src/ckeditor/ckeditor.component.spec.ts

Lines 122 to 170 in dc423da

    
           [ { 
        
           	newConfig: undefined, 
        
           	msg: 'without config', 
        
           	warn: false 
        
           }, { 
        
           	newConfig: { extraPlugins: 'basicstyles,divarea,link' }, 
        
           	msg: 'config.extraPlugins defined as a string', 
        
           	warn: false 
        
           }, { 
        
           	newConfig: { extraPlugins: [ 'basicstyles', 'divarea', 'link' ] }, 
        
           	msg: 'config.extraPlugins defined as an array', 
        
           	warn: false 
        
           }, { 
        
           	newConfig: { removePlugins: 'basicstyles,divarea,link,divarea' }, 
        
           	msg: 'config.removePlugins defined as a string', 
        
           	warn: isDivarea 
        
           }, { 
        
           	newConfig: { removePlugins: [ 'basicstyles', 'divarea', 'link', 'divarea' ] }, 
        
           	msg: 'config.removePlugins defined as an array', 
        
           	warn: isDivarea 
        
           } ].forEach( ( { newConfig, msg, warn } ) => { 
        
           	describe( msg, () => { 
        
           		beforeEach( () => { 
        
           			config = newConfig; 
        
           		} ); 
        
           		it( `console ${warn ? 'should' : 'shouldn\'t'} warn`, () => { 
        
           			const spy = spyOn( console, 'warn' ); 
        
           			fixture.detectChanges(); 
        
           			return whenEvent( 'ready', component ).then( () => { 
        
           				warn 
        
           					? expect( spy ).toHaveBeenCalled() 
        
           					: expect( spy ).not.toHaveBeenCalled(); 
        
           			} ); 
        
           		} ); 
        
           		it( `editor ${ isDivarea ? 'should' : 'shouldn\'t' } use divarea plugin`, () => { 
        
           			fixture.detectChanges(); 
        
           			return whenEvent( 'ready', component ).then( ( { editor } ) => { 
        
           				isDivarea 
        
           					? expect( editor.plugins.divarea ).not.toBeUndefined() 
        
           					: expect( editor.plugins.divarea ).toBeUndefined(); 
        
           			} ); 
        
           		} ); 
        
           	} ); 
        
           } );

test suite. However, those tests should no longer validate this case, as we already fixed upstream CKEditor 4 issue which required using divarea plugin for integrations. I will extract the issue as a separate ticket, as we should clean up the production code also.

Proposed changelog entry

* [#128](https://github.com/ckeditor/ckeditor4-angular/issues/128): Improve the stability of `getEditorNamespace()` method.

Closes #121
Closes #129.
Closes #128.

f1ames · 2020-08-03T07:04:05Z

I suppose it closes #93 too?

For anyone merging it, I think we should add changelog entry too, since @jacekbogdanski improved (getEditorNamespace) which is a part of he production code. And for the backlog entry we need an issue 😂 🙈 ⛓️

jacekbogdanski · 2020-08-03T07:16:11Z

I suppose it closes #93 too?

Yes, of course, looks like I confused ticket numbers.

For anyone merging it, I think we should add changelog entry too, since @jacekbogdanski improved (getEditorNamespace) which is a part of he production code. And for the backlog entry we need an issue joy see_no_evil chains

I wasn't sure about that because this change doesn't seem to matter from user perspective. But indeed in case if someone changes CDN URL into an empty string she/he will get console error instead of breaking the whole angular app. Changelog entry won't hurt.

I've extracted ticket for updating failing tests / divarea leftovers: #127

jacekbogdanski · 2020-08-03T07:23:40Z

Actually, I checked it right now and I see that a user gets exactly the same error message after proposed changes and the app behaves the same. I'm for skipping changelog entry and separate issue for that.

f1ames · 2020-08-03T07:33:46Z

Actually, I checked it right now and I see that a user gets exactly the same error message after proposed changes and the app behaves the same. I'm for skipping changelog entry and separate issue for that.

Good point, I was thinking more about the improvement itself and that it can solve some real-live edge cases (since now we clear and reject promise correctly, which could cause some issues earlier), so it might be useful info for developers anyway (and additional changelog entry indicates that we are working actively on the integration which is good sign for developers too). WDYT?

Dumluregn

Great job fixing the tests! They start to make sense when you look at them 🎉 Now let's polish this PR a little 😈 Besides the comments in code, please take a look below.

In ckeditor.helpers.spec.ts file we ignore one of the tests for IE10 - fortunately we officially don't support integration with this browser, so this check together with helpers can be removed.
There is a double space in L386 that could be removed.
PR lacks the change of karma.conf.js removing the unrandomising snippet:

jasmine: {
	random: false
}

After all I agree with @f1ames that it would be nice to have a changelog entry, mostly because we rarely have an opportunity to put there something different than updating CDN link or deps 🙈 I'll create an issue for 'Improving the stability of getEditorNamespace() function' and let's put it in changelog 🙂

src/ckeditor/ckeditor.component.spec.ts

src/ckeditor/ckeditor.helpers.spec.ts

src/ckeditor/ckeditor.component.spec.ts

Dumluregn · 2020-08-04T15:04:10Z

Created #128 to provide a changelog entry here.

jacekbogdanski · 2020-08-05T07:50:22Z

PR lacks the change of karma.conf.js removing the unrandomising snippet:

I'm not sure about that. Test still sometimes fails, probably also because of #127

I propose to do that once we fix #127 and we are sure that CI will be green with random order.

Dumluregn · 2020-08-05T12:21:20Z

There was something wrong with the commit history, so I re-rebased this branch onto latest master.

Dumluregn

I'm not sure about that. Test still sometimes fails, probably also because of #127
I propose to do that once we fix #127 and we are sure that CI will be green with random order.

Well, the console.warn tests are failing because you fixed them 😄 So CI will be red for now no matter if you run tests in order or randomly. So are we good with adding this Karma config? Or you meant something different?

jacekbogdanski · 2020-08-05T12:55:37Z

Well, the console.warn tests are failing because you fixed them smile So CI will be red for now no matter if you run tests in order or randomly. So are we good with adding this Karma config? Or you meant something different?

Yeah, they are failing despite random order, so let's make it random by default.

Dumluregn

LGTM!

Dumluregn

It seems I was happy to early 😞 CI fails not only because of the console.warn tests - on FF all tests fail 🦊 😿 (https://travis-ci.org/github/ckeditor/ckeditor4-angular/builds/715141554).

@jacekbogdanski please take a look what is going on there.

jacekbogdanski · 2020-08-07T09:00:19Z

We come to the conclusion that it takes too much time to fix running tests in random order. Instead, we will just focus on improving overall asynchronous test correctness proposed in this PR. I've changed Closes, so it will close correct issues. I've also reverted changes to karma config.

Dumluregn

Nice job and pleasure working on this one, I see it took us 4 days but feels like really a lot was done here 👍
It's also kind of a strange feeling when we close 3 tickets but none of them is the one that was meant to be closed in the first place 😄

Dumluregn self-assigned this Aug 4, 2020

Dumluregn self-requested a review August 4, 2020 12:53

Dumluregn requested changes Aug 4, 2020

View reviewed changes

src/ckeditor/ckeditor.component.spec.ts Outdated Show resolved Hide resolved

src/ckeditor/ckeditor.component.spec.ts Show resolved Hide resolved

src/ckeditor/ckeditor.helpers.spec.ts Outdated Show resolved Hide resolved

src/ckeditor/ckeditor.component.spec.ts Show resolved Hide resolved

CKEditorBot unassigned Dumluregn Aug 4, 2020

jacekbogdanski self-assigned this Aug 5, 2020

jacekbogdanski requested a review from Dumluregn August 5, 2020 07:50

CKEditorBot unassigned jacekbogdanski Aug 5, 2020

Dumluregn self-assigned this Aug 5, 2020

jacekbogdanski added 6 commits August 5, 2020 14:18

Fixed helpers tests.

3996430

Refactoring.

bf42cd1

Update config before each test case, not once for all.

02b7ebc

Removed ie10 sniffing.

3ed30fd

Replaced invalid space.

cfd11b1

Removed unnecessary asynchronous code.

35cb0ac

Dumluregn force-pushed the t/121-b branch from 685e62a to 35cb0ac Compare August 5, 2020 12:20

Dumluregn requested changes Aug 5, 2020

View reviewed changes

CKEditorBot unassigned Dumluregn Aug 5, 2020

Allow random test order.

89f4005

jacekbogdanski requested a review from Dumluregn August 5, 2020 12:54

Dumluregn approved these changes Aug 5, 2020

View reviewed changes

Dumluregn self-requested a review August 5, 2020 14:20

Dumluregn requested changes Aug 5, 2020

View reviewed changes

jacekbogdanski self-assigned this Aug 6, 2020

Reverse karma change.

5e67c8b

jacekbogdanski removed their assignment Aug 7, 2020

jacekbogdanski requested a review from Dumluregn August 7, 2020 09:00

Dumluregn approved these changes Aug 7, 2020

View reviewed changes

Dumluregn added 3 commits August 7, 2020 11:21

Fix whitespaces in repo.

34fe203

Remove legacy function 'mockPasteEvent()'.

e3119b1

Add changelog entry [skip ci].

a43fff7

Dumluregn changed the title ~~Fix running tests in a random order~~ Improve stability of package and tests Aug 7, 2020

Dumluregn merged commit ebf6aed into master Aug 7, 2020

Dumluregn deleted the t/121-b branch August 7, 2020 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve stability of package and tests #126

Improve stability of package and tests #126

jacekbogdanski commented Aug 3, 2020 •

edited

Loading

f1ames commented Aug 3, 2020

jacekbogdanski commented Aug 3, 2020 •

edited

Loading

jacekbogdanski commented Aug 3, 2020

f1ames commented Aug 3, 2020

Dumluregn left a comment

Dumluregn commented Aug 4, 2020

jacekbogdanski commented Aug 5, 2020

Dumluregn commented Aug 5, 2020

Dumluregn left a comment

jacekbogdanski commented Aug 5, 2020

Dumluregn left a comment

Dumluregn left a comment

jacekbogdanski commented Aug 7, 2020

Dumluregn left a comment

	[ {
	newConfig: undefined,
	msg: 'without config',
	warn: false
	}, {
	newConfig: { extraPlugins: 'basicstyles,divarea,link' },
	msg: 'config.extraPlugins defined as a string',
	warn: false
	}, {
	newConfig: { extraPlugins: [ 'basicstyles', 'divarea', 'link' ] },
	msg: 'config.extraPlugins defined as an array',
	warn: false
	}, {
	newConfig: { removePlugins: 'basicstyles,divarea,link,divarea' },
	msg: 'config.removePlugins defined as a string',
	warn: isDivarea
	}, {
	newConfig: { removePlugins: [ 'basicstyles', 'divarea', 'link', 'divarea' ] },
	msg: 'config.removePlugins defined as an array',
	warn: isDivarea
	} ].forEach( ( { newConfig, msg, warn } ) => {
	describe( msg, () => {
	beforeEach( () => {
	config = newConfig;
	} );

	it( `console ${warn ? 'should' : 'shouldn\'t'} warn`, () => {
	const spy = spyOn( console, 'warn' );

	fixture.detectChanges();

	return whenEvent( 'ready', component ).then( () => {
	warn
	? expect( spy ).toHaveBeenCalled()
	: expect( spy ).not.toHaveBeenCalled();
	} );
	} );

	it( `editor ${ isDivarea ? 'should' : 'shouldn\'t' } use divarea plugin`, () => {
	fixture.detectChanges();

	return whenEvent( 'ready', component ).then( ( { editor } ) => {
	isDivarea
	? expect( editor.plugins.divarea ).not.toBeUndefined()
	: expect( editor.plugins.divarea ).toBeUndefined();
	} );
	} );
	} );
	} );

Improve stability of package and tests #126

Improve stability of package and tests #126

Conversation

jacekbogdanski commented Aug 3, 2020 • edited Loading

Proposed changelog entry

f1ames commented Aug 3, 2020

jacekbogdanski commented Aug 3, 2020 • edited Loading

jacekbogdanski commented Aug 3, 2020

f1ames commented Aug 3, 2020

Dumluregn left a comment

Choose a reason for hiding this comment

Dumluregn commented Aug 4, 2020

jacekbogdanski commented Aug 5, 2020

Dumluregn commented Aug 5, 2020

Dumluregn left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Aug 5, 2020

Dumluregn left a comment

Choose a reason for hiding this comment

Dumluregn left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Aug 7, 2020

Dumluregn left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Aug 3, 2020 •

edited

Loading

jacekbogdanski commented Aug 3, 2020 •

edited

Loading